Cleanup GC mode switching in the interpreter - #132468
Conversation
The interpreter uses GCX_COOP_NO_DTOR / GCX_PREEMP_NO_DTOR when calling compiled methods with SEH wrapper / unmanaged methods. Due to that, it needed to have forceful restoration of cooperative mode in the catch for ResumeAfterCatchException. This change switches those usages to GCX_COOP() / GCX_PREEMP() instead. That removes the need to switch the GC mode explicitly in that catch. So I've replaced it by assert.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli |
|
/azp run runtime-interpreter |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR refactors GC mode switching in the CoreCLR interpreter’s SEH/unmanaged-call paths by replacing GCX_COOP_NO_DTOR / GCX_PREEMP_NO_DTOR usage with RAII-based GCX_COOP() / GCX_PREEMP(), and correspondingly simplifying the ResumeAfterCatchException catch path.
Changes:
- Introduces a helper to rethrow the thread’s last thrown managed exception from SEH handlers.
- Routes unmanaged transitions through a wrapper that uses
GCX_PREEMP()instead of_NO_DTORvariants. - Replaces explicit cooperative restoration in the
ResumeAfterCatchExceptioncatch with an assertion.
Suppressed comments (1)
src/coreclr/vm/interpexec.cpp:483
InvokeUnmanagedMethodnow creates aGCX_PREEMPholder (unwinding object) but is invoked from inside aPAL_TRY(__tryon Windows). If this function gets inlined into thatPAL_TRYframe under optimization/LTCG, it can reintroduce the VC++ restriction on mixing__trywith objects that require unwinding. Mark this helperNOINLINEto keep the GC-mode RAII out of thePAL_TRYfunction body.
void InvokeUnmanagedMethod(MethodDesc *targetMethod, int8_t *pArgs, int8_t *pRet, PCODE callTarget)
{
WRAPPER_NO_CONTRACT;
GCX_PREEMP();
InvokeManagedMethod(targetMethod, pArgs, pRet, callTarget, NULL);
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/azp run runtime-interpreter |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/interpexec.cpp:490
InvokeUnmanagedMethodInPreemptiveModeis invoked from within aPAL_TRYbody (which expands to__tryinsrc/coreclr/inc/palclr.h). If this helper gets inlined under optimization, theGCX_PREEMP()RAII object can end up inside the__tryregion, which is a pattern the SEH-basedPAL_TRYmacros are designed to avoid. Mark this helperNOINLINEto make the separation robust across configurations/compilers.
void InvokeUnmanagedMethodInPreemptiveMode(MethodDesc *targetMethod, int8_t *pArgs, int8_t *pRet, PCODE callTarget)
{
WRAPPER_NO_CONTRACT;
GCX_PREEMP();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/vm/interpexec.cpp:242
RethrowLastThrownObject()usesGCX_COOP()(RAII) before callingDispatchManagedException().CallWithSEHWrapper()is invoked under an outerGCX_PREEMP()(e.g.,PrepareInterpreterCode), so if this path is entered while preemptive and the exception propagates via C++ unwinding (the PAL_TRY implementation on non-Windows usestry/catch), theGCCoopdestructor can restore preemptive mode while the managed exception is being dispatched. This risks reintroducing the GC-mode mismatch the change is trying to eliminate. UseGCX_COOP_NO_DTOR()here since the helper isDECLSPEC_NORETURNand should not restore the incoming mode.
NOINLINE static void DECLSPEC_NORETURN RethrowLastThrownObject()
{
WRAPPER_NO_CONTRACT;
GCX_COOP();
OBJECTREF ohThrowable = GetThread()->LastThrownObject();
DispatchManagedException(ohThrowable);
|
/azp run runtime-interpreter |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Looks like there is a case when the new assert fires (the GC mode is not restored correctly). I need to investigate it. |
|
@davidwrighton the issue I am hitting with this change in the JIT/Interpreter test with tiered compilation off in the CI for this change is exactly what you were looking into. In this case, it is a QCALL to IsInstanceOf_NoCacheLookup that ends up throwing. It happens on Windows x64 too. |
|
@janvorli, Great! Now I've got an easier to debug repro case. |
The interpreter uses GCX_COOP_NO_DTOR / GCX_PREEMP_NO_DTOR when calling compiled methods with SEH wrapper / unmanaged methods. Due to that, it needed to have forceful restoration of cooperative mode in the catch for ResumeAfterCatchException.
This change switches those usages to GCX_COOP() / GCX_PREEMP() instead. That removes the need to switch the GC mode explicitly in that catch. So I've replaced it by assert.